Skip to content
This repository was archived by the owner on Jan 14, 2024. It is now read-only.

Manchester NW5-LeilaFaez-Js-Core1-Week1 - #371

Open
leilafaez wants to merge 4 commits into
CodeYourFuture:masterfrom
leilafaez:master
Open

Manchester NW5-LeilaFaez-Js-Core1-Week1#371
leilafaez wants to merge 4 commits into
CodeYourFuture:masterfrom
leilafaez:master

Conversation

@leilafaez

Copy link
Copy Markdown

Volunteers: Are you marking this coursework? You can find a guide on how to mark this coursework in HOW_TO_MARK.md in the root of this repository

Your Details

  • Your Name:Leila Faez
  • Your City:Manchester
  • Your Slack Name:leilafaez

Homework Details

  • Module:Javascript
  • Week:1

Notes

  • What did you find easy?

  • What did you find hard?

  • What do you still not understand?

  • Any other notes?


return "The total is total";
return "The total is " +total;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have solved the exercises and mandatory problems in a very neat, simple and understandable code and it's great. Well done.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks :)

@levilu800b levilu800b left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really enjoyed reviewing your code. The simplicity and the indentation make it easier for people or anyone else to read and understand.

function concatenate(firstWord, secondWord, thirdWord) {
// Write the body of this function to concatenate three words together.
// Look at the test case below to understand what this function is expected to return.
return firstWord + " " + secondWord + " " + thirdWord;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello Leila, you have done an amazing job and your codes are simple and easy to understand and review. However, I think for the return you could have used .concat to concatenate all the words together like on the above combine2Words function. If there is a reason for not using that do you mind explaining? Thank you

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your feedback. Yes you are completely right. I just wanted to try every piece of code that we have learnt during course :)

Comment thread mandatory/4-tax.js

@Gevie Gevie left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Leila, all in all a very good effort for the first JS week.

I've left mostly suggestions and feedback for improvements or alternative ways of doing things. Keep up the good work

  • Stephen

- What happens when you get rid of the quote marks?
- What happens when you console.log() just a number without quotes?
- What happens when you get rid of the quote marks? Face with SyntaxError(missing ) after argument list)
- What happens when you console.log() just a number without quotes? Face with SyntaxError(missing ) after argument list)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Leila

You're very close with the answer, the last answer isn't quite correct but I definitely see your logic for this one, you can try to run console.log(10); and see the result 👍

@@ -1,3 +1,5 @@
// Start by creating a variable `greeting`

let greeting="Hi Leila"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work not using var here! The greeting can actually be a const which is short for "constant" meaning the value never changes.

There will be other areas in this PR where const should be used instead of let or var but since the exercises didn't help with this - I won't mention it further on and you don't need to action it 👍

let messageType=typeof message;
console.log(message);
console.log(messageType);
// console.log(message + " "+ typeof message);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works great.

JavaScript does allow you to forget semicolons but we should always add them (line 2), no doubt this was an innocent mistake whilst learning :) I'd always suggest adding spaces between the variable and its assignment just for cleanliness / readability as well, like such (This feedback applies for the full PR):

const message = "this is a string";
const messageType = typeof message;

Finally one other suggestion would be to delete commented out code, it's a very easy thing to leave commented out code in your commits, I do it sometimes in my full time job as well. Here's a link to our style guide on commented out code to explain a little more: https://syllabus.codeyourfuture.io/guides/code-style-guide#dont-leave-lots-of-commented-out-code

console.log(message);
let name="Leila";
let message=name.length;
console.log("My name is"+" "+name + " "+ "and my name is"+" " + message+" "+ "character long.");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This passes and does exactly what we need it to do, great work Leila.

We could tidy it up a little, if you notice you are using concatenation to add a space +" "+ after a string. We could just add the space to the string before it instead, for example:

console.log("My name is " + name + " and my name is " + message + " characters long.");

By adding the spaces in the original strings, we have less + symbols and the line becomes a little easier to read.


console.log(message);
const message=name.length;
console.log("My name is" + " " + name.trim() + " " + "and my name is" + " " + message + " " +"character long.");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great effort, I would apply my previous feedback here regarding the spaces, just so it's a little easier to read. I won't comment any other instances of this suggestion in the PR :)

console.log("My name is " + name.trim() + " and my name is " + message + " character long.");

Comment thread extra/3-magic-8-ball.js
console.log("The ball has shaken!");
randomNumber = Math.floor(Math.random() * fixAnswer.length);
result =fixAnswer[randomNumber];
return result;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct and you've done well Leila, good use of the math library as well 👍

I would just be careful of your indentation as well as use const or let on your new variables, the return line is indented more than the other lines.

Another way you could return your value is just passing fixAnswer[randomNumber] instead of using the variable since it's quite a small line.

function shakeBall() {
  console.log("The ball has shaken!");
  const randomNumber = Math.floor(Math.random() * fixAnswer.length);
  
  return fixAnswer[randomNumber];
}

Comment thread extra/3-magic-8-ball.js
answer = "negative";
} else if (ballPredict <= 19) {
answer = "very negative";
} else answer = "Something broken";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Leila, you've pretty much hit the nail on the head here 👍

I would just be careful at the end with your } else answer = "Something broken"; as this syntax isn't quite correct. I also notice you define answer then return it at the end, another way of doing this is just to return in each conditional block. For example:

function checkAnswer(answer) {
  const ballPredict = fixAnswer.indexOf(answer);
  
  if (ballPredict <= 4) {
    return "very positive";
  } else if (ballPredict <= 9) {
    return "positive";
  } else if (ballPredict <= 14) {
    return "negative";
  } else if (ballPredict <= 19) {
    return "very negative";
  }
  
  return "Something broken";
}


return word.trim();
}
word = " CodeYourFuture ";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have successfully completed the exercise by fixing the errors 💯

We are however defining a word variable 4 times in this file, reassigning its value and never actually using the variable anywhere. We can probably delete these lines :)

// Look at the test case below to understand what this function is expected to return.
return firstWord + " " + secondWord + " " + thirdWord;
}
firstWord="code";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've completed this mandatory exercise spot on Leila :)

The only feedback would be that we don't need the firstWord, secondWord and thirdWord variables on line 19 to 21 since they're never used 👍

Comment thread mandatory/4-tax.js

function calculateSalesTax() {}
function calculateSalesTax(price) {
const saleTax = (price * 20) / 100;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job, there's nothing I can suggest for improvements here :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants